Refactor: Modularize architecture#34
Conversation
… rendering support - Extract ArrayModel from controllers for improved separation of concerns - Introduce Strategy pattern for shuffle and delay mechanisms - Refactor rendering system with RenderContext abstraction: - Add HeadlessRenderContext for non-GUI mode - Add ProcessingContext for unified processing management - Support both graphical and headless rendering - Add HeadlessSound implementation for audio-only mode - Migrate to design patterns for better extensibility - Add comprehensive test coverage for new Control layer abstractions - Update across all 60+ source files with improved modularity and reusability - Enhance testability with new test suites for ArrayModel and headless components
There was a problem hiding this comment.
Pull request overview
Refactors the sorting visualizer toward a more modular architecture by introducing ArrayModel, strategy abstractions (shuffle + delay), and a RenderContext/ProcessingContext split intended to enable headless operation and improved testability.
Changes:
- Introduce
ArrayModel,ProcessingContext,RenderContext, andDelayStrategyto decouple algorithms/visuals from Processing (PApplet). - Extract shuffle behaviors into
ShuffleStrategyimplementations and updateArrayControllerto delegate shuffling. - Add headless implementations/tests (
HeadlessRenderContext,HeadlessSound) and update build/run packaging to a thin jar + copied runtime dependencies (fat jar viareleaseprofile).
Reviewed changes
Copilot reviewed 82 out of 82 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/io/github/compilerstuck/SortingAlgorithms/SortingAlgorithmsTest.java | Updates algorithm ctor reflection to use ArrayModel; adapts Processing stub to ProcessingContext. |
| src/test/java/io/github/compilerstuck/Control/HeadlessVisualizationAndSoundTest.java | Adds headless smoke tests for basic visuals and HeadlessSound. |
| src/test/java/io/github/compilerstuck/Control/ArrayModelAndStrategyTest.java | Adds unit tests for ArrayController behavior, delay strategy, and shuffle strategies. |
| src/main/java/io/github/compilerstuck/Visual/Visualization.java | Switches visuals to depend on ArrayModel + RenderContext instead of ArrayController + PApplet. |
| src/main/java/io/github/compilerstuck/Visual/SwirlDots.java | Updates ctor/signatures for ArrayModel/RenderContext; introduces PApplet downcast usage. |
| src/main/java/io/github/compilerstuck/Visual/SphericDisparityLines.java | Updates rendering to RenderContext with multiple PApplet downcasts for 3D APIs. |
| src/main/java/io/github/compilerstuck/Visual/SphereHoops.java | Updates rendering to RenderContext with multiple PApplet downcasts for 3D APIs. |
| src/main/java/io/github/compilerstuck/Visual/Sphere.java | Updates rendering to RenderContext with multiple PApplet downcasts for 3D APIs. |
| src/main/java/io/github/compilerstuck/Visual/ScatterPlotLinked.java | Updates ctor/signatures for ArrayModel/RenderContext. |
| src/main/java/io/github/compilerstuck/Visual/ScatterPlot.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for drawing. |
| src/main/java/io/github/compilerstuck/Visual/Pyramid.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for 3D APIs. |
| src/main/java/io/github/compilerstuck/Visual/Plane.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for 3D APIs. |
| src/main/java/io/github/compilerstuck/Visual/Phyllotaxis.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for circle drawing. |
| src/main/java/io/github/compilerstuck/Visual/NumberPlot.java | Updates ctor/signatures for ArrayModel/RenderContext; adjusts text call to pass String. |
| src/main/java/io/github/compilerstuck/Visual/MosaicSquares.java | Updates ctor/signatures for ArrayModel/RenderContext. |
| src/main/java/io/github/compilerstuck/Visual/MorphingShell.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for 3D APIs. |
| src/main/java/io/github/compilerstuck/Visual/ImageVertical.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for pixel APIs. |
| src/main/java/io/github/compilerstuck/Visual/ImageHorizontal.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for pixel APIs. |
| src/main/java/io/github/compilerstuck/Visual/HorizontalPyramid.java | Updates ctor/signatures for ArrayModel/RenderContext; switches to proc.getHeight(). |
| src/main/java/io/github/compilerstuck/Visual/Hoops.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for noFill(). |
| src/main/java/io/github/compilerstuck/Visual/DisparitySquareScatter.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for circle/noStroke. |
| src/main/java/io/github/compilerstuck/Visual/DisparitySphereHoops.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for 3D APIs. |
| src/main/java/io/github/compilerstuck/Visual/DisparityPlane.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for 3D APIs. |
| src/main/java/io/github/compilerstuck/Visual/DisparityGraphMirrored.java | Updates ctor/signatures for ArrayModel/RenderContext; switches to proc.getHeight(). |
| src/main/java/io/github/compilerstuck/Visual/DisparityGraph.java | Updates ctor/signatures for ArrayModel/RenderContext. |
| src/main/java/io/github/compilerstuck/Visual/DisparityCircleScatterLinked.java | Updates ctor/signatures for ArrayModel/RenderContext. |
| src/main/java/io/github/compilerstuck/Visual/DisparityCircleScatter.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for circle/noStroke. |
| src/main/java/io/github/compilerstuck/Visual/DisparityCircle.java | Updates ctor/signatures for ArrayModel/RenderContext. |
| src/main/java/io/github/compilerstuck/Visual/DisparityChords.java | Updates ctor/signatures for ArrayModel/RenderContext. |
| src/main/java/io/github/compilerstuck/Visual/CubicLines.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for 3D APIs. |
| src/main/java/io/github/compilerstuck/Visual/Cube.java | Updates ctor/signatures for ArrayModel/RenderContext; uses PApplet downcasts for 3D APIs. |
| src/main/java/io/github/compilerstuck/Visual/ColorGradientGraph.java | Updates ctor/signatures for ArrayModel/RenderContext. |
| src/main/java/io/github/compilerstuck/Visual/Circle.java | Updates ctor/signatures for ArrayModel/RenderContext. |
| src/main/java/io/github/compilerstuck/Visual/Bars.java | Updates ctor/signatures for ArrayModel/RenderContext. |
| src/main/java/io/github/compilerstuck/Sound/Sound.java | Switches sound base class to depend on ArrayModel. |
| src/main/java/io/github/compilerstuck/Sound/MinimSound.java | Updates ctor to accept ArrayModel; casts MainController.processing back to PApplet. |
| src/main/java/io/github/compilerstuck/Sound/MidiSys.java | Updates ctor to accept ArrayModel; casts MainController.processing back to PApplet. |
| src/main/java/io/github/compilerstuck/Sound/HeadlessSound.java | Adds a no-op sound implementation for headless/tests. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/TimSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/SortingAlgorithm.java | Refactors to ArrayModel + ProcessingContext; introduces DelayStrategy injection and delay-factor setter. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/ShellSort.java | Updates ctor to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/ShakerSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/SelectionSort.java | Updates ctor to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/RadixLSDSortBase10.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/QuickSortMiddlePivot.java | Updates ctor and helper signature to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/QuickSortDualPivot.java | Updates ctors and helper signature to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/PigeonholeSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/OddEvenSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/MergeSort.java | Updates ctor and helpers to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/InsertionSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/HeapSort.java | Updates ctor and helper signature to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/GravitySort.java | Updates ctor to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/GnomeSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/DoubleSelectionSort.java | Updates ctor to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/CycleSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/CountingSort.java | Updates ctor to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/CombSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/BucketSort.java | Updates ctor to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/BubbleSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/BogoSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/SortingAlgorithms/AmericanFlagSort.java | Updates ctors to accept ArrayModel. |
| src/main/java/io/github/compilerstuck/Control/shuffle/SortedShuffleStrategy.java | Adds sorted shuffle strategy implementation. |
| src/main/java/io/github/compilerstuck/Control/shuffle/ReverseShuffleStrategy.java | Adds reverse shuffle strategy implementation. |
| src/main/java/io/github/compilerstuck/Control/shuffle/RandomShuffleStrategy.java | Adds random shuffle strategy implementation and delay checkpoint helper. |
| src/main/java/io/github/compilerstuck/Control/shuffle/AlmostSortedShuffleStrategy.java | Adds “almost sorted” shuffle strategy implementation. |
| src/main/java/io/github/compilerstuck/Control/ShuffleStrategy.java | Adds shuffle strategy interface. |
| src/main/java/io/github/compilerstuck/Control/Settings.java | Adds speed slider wiring delay time/factor; updates visualization construction to pass RenderContext. |
| src/main/java/io/github/compilerstuck/Control/RenderContext.java | Adds rendering abstraction over minimal Processing draw API. |
| src/main/java/io/github/compilerstuck/Control/ProcessingContext.java | Adds minimal Processing abstraction for delays. |
| src/main/java/io/github/compilerstuck/Control/MainController.java | Implements RenderContext; changes static processing type to ProcessingContext; adds delay setters. |
| src/main/java/io/github/compilerstuck/Control/JCheckBoxList.java | Replaces suppression with explicit serialVersionUID. |
| src/main/java/io/github/compilerstuck/Control/HeadlessRenderContext.java | Adds no-op render context for headless operation/tests. |
| src/main/java/io/github/compilerstuck/Control/DelayStrategy.java | Adds delay strategy abstraction with default probabilistic implementation. |
| src/main/java/io/github/compilerstuck/Control/ArrayModel.java | Adds array model interface for algorithms/UI instrumentation. |
| src/main/java/io/github/compilerstuck/Control/ArrayController.java | Implements ArrayModel; replaces shuffle-type branching with ShuffleStrategy delegation. |
| run_portrait.cmd | Updates Windows launch script to use classpath + main class. |
| run_fullscreen.cmd | Updates Windows launch script to use classpath + main class. |
| run.cmd | Updates Windows launch script to use classpath + main class. |
| run | Updates Unix launch script to use classpath + main class. |
| pom.xml | Switches default packaging to thin jar + copied deps; adds release profile for assembly fat jar. |
| build | Adds “fast” default build mode (skips tests) and a release mode for fat jar build. |
| README.md | Updates visuals count/list and documents speed control feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| MainController.setDelayTime(DELAY_TIME[level]); | ||
| MainController.setDelayFactor(DELAY_FACTOR[level]); |
There was a problem hiding this comment.
The speed slider updates only the algorithms currently registered in MainController.algorithms. In this app, Settings creates its own algorithmList of new SortingAlgorithm instances and later swaps them into MainController via setAlgorithm(...) / setAlgorithms(...), so newly selected algorithms may not inherit the user’s chosen speed settings. Consider storing the selected delay settings centrally (e.g., in MainController) and applying them inside setAlgorithm/setAlgorithms, or applying the slider values directly to Settings.algorithmList as well.
| MainController.setDelayTime(DELAY_TIME[level]); | |
| MainController.setDelayFactor(DELAY_FACTOR[level]); | |
| int delayTime = DELAY_TIME[level]; | |
| double delayFactor = DELAY_FACTOR[level]; | |
| // Update delay settings in the main controller | |
| MainController.setDelayTime(delayTime); | |
| MainController.setDelayFactor(delayFactor); | |
| // Also apply the selected speed to algorithms managed by Settings | |
| if (algorithmList != null) { | |
| for (SortingAlgorithm algorithm : algorithmList) { | |
| if (algorithm != null) { | |
| algorithm.setDelayTime(delayTime); | |
| algorithm.setDelayFactor(delayFactor); | |
| } | |
| } | |
| } |
| int y = screenHeight / 2 + (int) (radius * arrayController.get(i) / arrayController.getLength() * Math.cos(phase)); | ||
|
|
||
| proc.circle(x, y, 5); //Swirl dots | ||
| ((PApplet)proc).circle(x, y, 5); //Swirl dots |
There was a problem hiding this comment.
proc is a RenderContext, but this code casts it back to PApplet to call circle(). That will throw ClassCastException when a non-Processing RenderContext (e.g., HeadlessRenderContext) is used, undermining the headless rendering goal. Consider extending RenderContext with the drawing operations the visualizations need (e.g., circle) and implementing them in both MainController and HeadlessRenderContext, or splitting 2D/3D visuals behind separate context interfaces.
| model.swap(i, j); | ||
| model.setMarker(i, Marker.SET); | ||
| model.setMarker(j, Marker.SET); | ||
| MainController.setCurrentOperation("Shuffling (reverse).. " + (int) (i / (half - 1.) * 100) + "%"); |
There was a problem hiding this comment.
Progress percentage uses (half - 1.) as the divisor. When length is 2 (so half == 1), this becomes a division by zero and results in an infinite percentage (casts to Integer.MAX_VALUE), producing bogus UI output. Consider guarding small lengths (e.g., treat 0%/100% specially, or use Math.max(1, half - 1) as the denominator).
| MainController.setCurrentOperation("Shuffling (reverse).. " + (int) (i / (half - 1.) * 100) + "%"); | |
| MainController.setCurrentOperation("Shuffling (reverse).. " + (int) (i * 100.0 / Math.max(1, half - 1)) + "%"); |
| /** | ||
| * Fisher-Yates (random) shuffle. | ||
| */ | ||
| public class RandomShuffleStrategy implements ShuffleStrategy { | ||
|
|
||
| @Override | ||
| public void shuffle(ArrayModel model, ProcessingContext ctx) { | ||
| int length = model.getLength(); | ||
| for (int i = 0; i < length && SortingAlgorithm.isRun(); i++) { | ||
| int j = (int) (Math.random() * length); | ||
| model.swap(i, j); | ||
| model.setMarker(i, Marker.SET); |
There was a problem hiding this comment.
The class-level Javadoc claims this is a Fisher–Yates shuffle, but the implementation picks j uniformly from [0, length) for every i, which is not Fisher–Yates and does not produce a uniform permutation. Either update the implementation to a real Fisher–Yates (choose j from [i, length)) or adjust the documentation to match the actual behavior.
| model.setMarker(j, Marker.SET); | ||
| MainController.setCurrentOperation("Shuffling.. " + (int) ((double) i / (length - 1) * 100) + "%"); | ||
| maybeDelay(ctx, i, length); |
There was a problem hiding this comment.
Progress percentage divides by (length - 1). For length <= 1, this produces Infinity/NaN and can display nonsensical values (casts to Integer.MAX_VALUE). Consider guarding small lengths (e.g., treat length==1 as 100% and skip the division).
| model.setMarker(i, Marker.SET); | ||
| MainController.setCurrentOperation("Shuffling (sorted).. " + (int) ((double) i / (length - 1) * 100) + "%"); | ||
| RandomShuffleStrategy.maybeDelay(ctx, i, length); |
There was a problem hiding this comment.
Progress percentage divides by (length - 1). For length <= 1, this yields Infinity/NaN and can display nonsensical values (or potentially propagate to other calculations). Consider guarding length <= 1 and reporting 100% immediately (or use Math.max(1, length - 1) as the denominator).
| model.swap(a, b); | ||
| model.setMarker(a, Marker.SET); | ||
| model.setMarker(b, Marker.SET); | ||
| MainController.setCurrentOperation("Shuffling (almost).. " + (int) ((double) i / (swaps - 1) * 100) + "%"); |
There was a problem hiding this comment.
Progress percentage divides by (swaps - 1). When length is 10–19, swaps becomes 1 and this divides by zero, producing an infinite percentage in the UI string. Handle the swaps <= 1 case explicitly (e.g., report 100% after the single swap, or clamp the denominator with Math.max(1, swaps - 1)).
| MainController.setCurrentOperation("Shuffling (almost).. " + (int) ((double) i / (swaps - 1) * 100) + "%"); | |
| int progress = (swaps <= 1) ? 100 : (int) ((double) i / (swaps - 1) * 100); | |
| MainController.setCurrentOperation("Shuffling (almost).. " + progress + "%"); |
| void headlessSoundNoOp() { | ||
| sound.playSound(5); | ||
| // no exception and nothing to verify - simply ensure method exists | ||
| assertTrue(true); | ||
| } |
There was a problem hiding this comment.
assertTrue(true) doesn't validate anything and can be removed. If the intent is to verify HeadlessSound is a no-op, use assertDoesNotThrow(() -> sound.playSound(5)) (and optionally cover mute(...) similarly).
| <configuration> | ||
| <archive> | ||
| <manifest> | ||
| <mainClass>io.github.compilerstuck.Control.MainController</mainClass> |
There was a problem hiding this comment.
The main build now produces a thin target/sorting-visualizer.jar plus copied dependencies, but the JAR manifest doesn’t include a Class-Path, so java -jar target/sorting-visualizer.jar will not find runtime deps unless users use the provided -cp ... dependency/* launch pattern. If you want the jar to remain runnable via java -jar, consider adding a Class-Path manifest entry (pointing at dependency/*) or using a shading/assembly approach for the default build.
| <mainClass>io.github.compilerstuck.Control.MainController</mainClass> | |
| <mainClass>io.github.compilerstuck.Control.MainController</mainClass> | |
| <addClasspath>true</addClasspath> | |
| <classpathPrefix>dependency/</classpathPrefix> |
| <appendAssemblyId>true</appendAssemblyId> | ||
| <descriptorRefs> | ||
| <descriptorRef>jar-with-dependencies</descriptorRef> | ||
| </descriptorRefs> | ||
| </configuration> |
There was a problem hiding this comment.
In the release profile, appendAssemblyId is true, so the fat jar will be named sorting-visualizer-jar-with-dependencies.jar (while target/sorting-visualizer.jar remains the thin jar). If release automation / download links expect the runnable artifact to be sorting-visualizer.jar, consider setting appendAssemblyId=false or otherwise aligning the produced filename with the expected release asset name.
Modularize architecture with strategy patterns and headless rendering support